Skip to content

feat(imap): add repair-labels command - #754

Merged
wesm merged 6 commits into
kenn-io:mainfrom
local-ha:feat/imap-repair-labels
Sep 5, 2026
Merged

feat(imap): add repair-labels command#754
wesm merged 6 commits into
kenn-io:mainfrom
local-ha:feat/imap-repair-labels

Conversation

@exactmike

@exactmike exactmike commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Closes #748

Adds repair-labels, shaped like repair-encoding, repair-senders,
repair-dates, and repair-list-ids. It rebuilds an IMAP source's message
labels from its stored imap_message_memberships rows on demand — the same
rebuild a full mailbox enumeration performs, without paying for one on every
sync.

It only rebuilds labels. A message with no membership rows never enters the
repair, so tombstoning stays a full-enumeration concern, and the default is
still a dry run.

On the 118,000-message Microsoft 365 account from #748, a manually reproduced
stray label (an add-only merge with no backing membership row) was found and
removed in a ~4s --apply run, leaving the message's real label untouched.

No configuration or usage changes.

An add-only label merge (ReconcileMessageLabels with replace=false) never
removes a label, and nothing later revisits a message unless its stored
membership row changes again — a full mailbox enumeration is what used to
clean this up, but PR kenn-io#699 made that expensive to run on every sync.

repair-labels rebuilds a source's message_labels straight from
imap_message_memberships on demand, reusing the same rebuild a full
enumeration performs. Shaped like repair-encoding, repair-senders,
repair-dates, and repair-list-ids: dry run by default, --apply to write,
plus an optional identifier to scope to one source.

Closes kenn-io#748.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DhzbNgimfaBhPUSMzB8v2A
@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (b6a6f1f)

Verdict: One medium-severity issue found in the optional source argument handling.

Medium

  • Location: cmd/msgvault/cmd/repair_labels.go:67-73
  • Problem: The optional source argument is matched only against src.Identifier. Real IMAP sources use canonical identifiers such as imaps://..., while documented email arguments correspond to the source display name. This can silently scan zero messages and report success.
  • Fix: Match against both identifier and display name, and return an error when no IMAP source matches.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 8m11s

A source's Identifier is its full imaps://user@host:993 connection
string, not the email address a person would type — repair-labels'
optional identifier argument matched only that, so the documented
usage (an email address) silently matched zero sources and reported
success. Use sourceops.ResolveExactOne, the same identifier-or-display-
name resolver remove-account and repair-identity already use, and
return the standard "no account found" error instead of going quiet.

Found by roborev on PR kenn-io#754.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DhzbNgimfaBhPUSMzB8v2A
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (1af9e5f)

Verdict: One medium-severity issue requires attention.

  • Mediuminternal/store/imap_memberships.go:738-755: The repair accepts a context but uses non-context transaction helpers, so cancellation cannot interrupt long repairs. In dry-run mode, cancellation may also be masked by the intentional rollback sentinel being returned as success.
    • Fix: Thread ctx through all queries and writes, check it between messages, and return ctx.Err() when cancellation occurs.

Reviewers: 2 done | Synthesis: codex, 5s | Total: 5m43s

The repair took a context but never checked it: a long-running or
cancelled request would still process every message, and — more
subtly — the dry-run rollback sentinel returned as a nil error
unconditionally, so a cancellation landing near the end of the loop
would be reported as a completed dry run instead of surfaced as a
cancellation.

Checks ctx between messages and once more before the dry-run branch.
Because database/sql rolls back a transaction in a background
goroutine as soon as its context is cancelled, a query in flight can
lose that race and surface a raw driver error instead of a clean
ctx.Err() — normalized at the return boundary so callers always see
ctx.Err() when the context is actually done, regardless of which
statement the race landed on.

Found by roborev on PR kenn-io#754.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DhzbNgimfaBhPUSMzB8v2A
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (a1e96e9)

Verdict: One medium-severity issue identified; otherwise, no concrete security concerns found.

Medium

  • cmd/msgvault/cmd/repair_labels.go:84-115 — With --apply across multiple sources, each source commits independently, but cache rebuilding occurs only after the entire loop succeeds. If a later repair, output write, or context cancellation fails, earlier repairs remain committed while the derived analytics cache is stale. Track committed changes and rebuild the cache on partial failure, or make the repair and cache refresh atomic from the command’s perspective.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 6m55s

…failure

Each IMAP source repairs and commits independently, but the cache
rebuild only ran after the whole multi-source loop returned without
error. A later source failing — another source's repair, an output
write, or a cancelled context — left every source repaired before it
committed with no cache rebuild to follow, leaving the analytics
cache stale with no signal to fix itself.

Accumulate totals as each source commits, run the loop in a closure,
and rebuild the cache unconditionally whenever --apply is set,
independent of whether the loop itself returned an error — mirroring
repair-identity's existing errors.Join(rerr, cacheErr) shape for the
same situation.

Found by roborev on PR kenn-io#754.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DhzbNgimfaBhPUSMzB8v2A
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (f4bde1d)

Verdict: One medium-severity issue found.

Medium

  • internal/store/imap_memberships.go:741-765 — The repair accepts a context but performs transactional queries and writes through wrappers using context.Background(), so cancellation is not propagated to in-flight database operations. Thread ctx through the repair helpers and use QueryContext, QueryRowContext, and ExecContext throughout the transaction.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 7m13s

The repair's own ctx.Err() polling between messages (added for the
previous roborev round) still left every individual statement running
through tx.Query/tx.Exec, which use context.Background() internally —
so a genuinely slow statement could not be interrupted mid-flight by
the driver, only noticed after it returned.

Threaded ctx through distinctIMAPMembershipMessageIDs,
imapMembershipMailboxes, and ensureIMAPMailboxLabel's lookup path —
the three call sites this touches (here and applyIMAPMailboxDeltas's
own rebuild loop) both already carry ctx. Left ensureLabelWith and
reconcileMessageLabelsTx on context.Background(): both are shared by
public methods (EnsureLabel, EnsureLabelsBatch, ReconcileMessageLabels,
AddMessageLabels) with no ctx in their own signatures, and threading
ctx through them belongs to a broader change than this command.

Found by roborev on PR kenn-io#754.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DhzbNgimfaBhPUSMzB8v2A
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (9ee581c)

Verdict: One medium-severity context-cancellation issue remains in the IMAP label-repair path.

Medium

  • internal/store/imap_memberships.go:689-694, 765: RepairIMAPSourceLabels is not fully context-aware. ensureLabelWith and reconcileMessageLabelsTx use background-context tx.Query/tx.Exec calls, so cancellation may not stop the repair or release its transaction promptly. Thread ctx through the helpers and use QueryContext/ExecContext throughout.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 9m59s

RepairIMAPSourceLabels polled ctx.Err() between messages, but the
statements underneath ran on context.Background(), so a cancellation
could not reach a query already in flight.

ensureLabelWith and reconcileMessageLabelsTx are shared with EnsureLabel,
EnsureLabelsBatch, ReconcileMessageLabels and AddMessageLabels, none of
which have a context to give. Rather than change those four signatures,
use the two patterns the store package already has for this: boundQuerier
carries ctx into a querier-taking helper, and a Context sibling carries it
through reconcileMessageLabelsTx while the old name keeps delegating with
context.Background().

No public signature changes.

Found by roborev on PR kenn-io#754.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HULapGKBxKDSNETGNhrGjG
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (02e84d9)

Verdict: One medium-severity cache invalidation issue identified; no security vulnerabilities found.

Medium

  • internal/store/imap_memberships.go:760-786ensureIMAPMailboxLabel may rename an existing label without setting Changed, so the derived-data revision is not bumped when label IDs already match. Analytics caches may retain the old label name. Track label metadata changes and invalidate the cache when labels are renamed or otherwise modified.

Reviewers: 2 done | Synthesis: codex, 5s | Total: 8m34s

@exactmike

Copy link
Copy Markdown
Contributor Author

ensureIMAPMailboxLabel cannot rename a label, so an exported name cannot go stale.

It reaches ensureLabelWith only after its own SELECT id FROM labels WHERE source_id = ? AND source_label_id = ? returns no row. ensureLabelWith opens with the same predicate on the same transaction, so it finds no row either, and its err == nil branch never runs. Every rename, every mergeLabelByName call, and every label_type update sits inside that branch. What runs instead is the insert with ON CONFLICT(source_id, name) DO UPDATE, which conflicts on the name and therefore never changes one.

The two remaining outcomes are both safe. A newly inserted label is attached to the message by the reconcile below, which reports a change and bumps the revision. An existing row adopted by name changes only source_label_id and label_type, and the analytics cache exports neither: it reads SELECT id, name FROM labels and SELECT message_id, label_id FROM message_labels.

Reaching either outcome from the repair needs a mailbox whose label row is missing, which a completed sync does not leave behind. applyIMAPMailboxDeltas creates the label in the same transaction that writes the membership rows the repair reads.

@wesm
wesm merged commit f3ee0f1 into kenn-io:main Sep 5, 2026
25 checks passed
@exactmike
exactmike deleted the feat/imap-repair-labels branch September 5, 2026 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Add-only IMAP label merges rely on a full enumeration to ever finish

2 participants